-
Notifications
You must be signed in to change notification settings - Fork 909
[cryptotest] ACVP test harness with HMAC-SHA2-256 vectors #28647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, this looks like a great addition to the crypto testing. I've left a couple of comments and suggestions/nits but the implementation looks good to me, and I'm happy to approve when CI is passing.
With regards to getting past CI, there is some guidance on the format of commit messages. These are mostly just suggestions, but there are two aspects that are generally enforced:
-
The commit message should contain a prefix describing the area/scope of the changes. This is entirely up to you, but my suggestion would be to
git commit --amendand change the first line to be[cryptotest] ACVP test harness with HMAC-SHA2-256 vectors
It would also be great if you could do the same with the PR title :)
-
We require that commits are signed-off to indicate agreement with the Contributor License Agreement (CLA) - see CONTRIBUTING.md for more information. To do that you can run:
git commit --amend --signoff --no-edit git push -f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick changes.
It would be great if you could add the [cryptotest] prefix to your commit message (see point 1 here), but other than that I'm happy.
Edit: It also looks like CI is complaining about trailing whitespaces. I think this should normally be handled by ./bazelisk.sh run format, but it looks like we don't have this for JSON files. You can run ./ci/scripts/whitespace.sh master to find trailing whitespace introduced in the diff from master, but given we already know the issue you can run:
./util/fix_trailing_whitespace.py sw/host/cryptotest/testvectors/acvp/hmac_sha256_expected.json
to fix the problem (alternatively, just manually fix the missing newline at the end of the file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Brian for the contribution!
All 600 test vectors are passing & the code looks good to me!
Once CI passes, we can merge this.
|
(I think it needs another |
Introduce test harness capable of running ACVP test vectors on the Cryptotest firmware. Test outputs can be written to a result file for uploading to the NIST demo server or another verification authority. Also supports comparing results against a golden reference. This initial version supports HMAC-SHA2, with test vectors and expected outputs that have been verified against the NIST demo server. Signed-off-by: Brian Orr <[email protected]>
Introduce test harness capable of running ACVP test vectors on the Cryptotest firmware. Test outputs can be written to a result file for uploading to the NIST demo server or another verification authority. Also supports comparing results against a golden reference.
This initial version supports HMAC-SHA2, with test vectors and expected outputs that have been verified against the NIST demo server.